Apple client send and receive support for crash report cohort IDs (CRCIDs) #1116
+309
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CrashCollection now supports dependency injection of both CrashReportSending and KeyValueStoring parameters.
… complete • CrashCollectionTests has a new test for CRCID use (first of a few planned) • CrashReportSender now returns data or an error • CrashReportSender is now testable (previously it’s async design made it impossible to test results of sending)
CrashReportSender and CrashCollection now support sending and recieving CRCIDs with a not-yet-finalized HTTP header value. One test present, and more planned out.
Tested against the dev server instance, and CRCIDs are sent and received as expected!
samsymons
reviewed
Dec 5, 2024
samsymons
reviewed
Dec 5, 2024
LarryTurtis
reviewed
Dec 5, 2024
Also cleaned up some logging messages
16 tasks
This enables reuse in the macOS app non-appstore builds (which don’t use CrashCollection).
|
Ok, I’ve updated BSK again so we can support crashes in the macOS app as well. To do this, I’ve created a new CRCIDManager class that is responsible for crcid storage and handling logic around updating, or not, the CRCID based on server response. This lets the macOS client (non-appstore) use the same logic without relying on CrashCollection, which is only used by macOS app store builds. |
This enables the server to only cohort and send CRCIDs back to clients that support CRCIDs, making rollout of this feature much simpler.
Empty CRCID header leads server to assign a CRCID. No header = no cohorting.
jdjackson
commented
Dec 16, 2024
jdjackson
force-pushed
the
jjackson/crcid-send-receive-support
branch
from
3249ac6 to
4fd8049
Compare
samsymons
reviewed
Dec 19, 2024
samsymons
reviewed
Dec 19, 2024
samsymons
reviewed
Dec 19, 2024
samsymons
reviewed
Dec 19, 2024
samsymons
reviewed
Dec 19, 2024
samsymons
reviewed
Dec 19, 2024
No longer need to have MockKeyValueStore inherit NSObject. And left explanatory comment about calls to crash.js needing to be sequential.
CRCID header value needs to be an empty string when we don't have a value yet, but KeyValueStoring is more appropriately used with optional values. We now support an optional value in CRCIDManager, and send an empty string with the header if we don't have one yet.
* main: (24 commits) change api (#1133) Ensure authToken is present before calling refreshAuthTokenIfNeeded Add 'locale' to report broken site params Add 'locale' to report broken site params Ensure authToken is present before calling refreshAuthTokenIfNeeded Privacy Pro Free Trials - Models and API (#1120) remove MaliciousSiteProtectionSubfeature (#1131) Malware protection 6: Malware integration (#1099) Add underlying error to PrivacyStatsError (#1130) Initial changes for compilation time tracking. (#1111) iOS System level credential provider (#1127) Increase ratio of complete form saves (#1124) Add PrivacyStatsError.failedToClearPrivacyStats (#1128) Update autofill to 16.0.0 (#1122) Update local network routing (#1117) Update RMF version matching to ignore build number (#1118) Fix threading issue between using a Semaphore and async/await (#1115) add experiment test fake feature (#1119) Bump Tests/BrowserServicesKitTests/Resources/privacy-reference-tests from `a603ff9` to `6133e7d` (#1109) experiment default metrics pixels (#1107) ...
samsymons
approved these changes
Dec 19, 2024
jdjackson
added a commit
to duckduckgo/iOS
that referenced
this pull request
Dec 19, 2024
Task/Issue URL: https://app.asana.com/0/1208592102886666/1208759541597499/f Tech Design URL: https://app.asana.com/0/1208592102886666/1208660326715650/f **Description**: DO NOT MERGE - this is a draft for input, not ready to go live yet. iOS client support for CRCID send/receive (primarily supported in BSK, with changes under review in [BSK #1116](duckduckgo/BrowserServicesKit#1116)). This is pretty straightforward, just conforming to CrashCollection’s new init signature, and clearing CRCIDs when the user opts out of crash reporting. BSK handles everything else. **Steps to test this PR**: Note: Must be tested on a physical device, as the simulator does not produce crash logs (and thus doesn’t find and upload them either). To cause and report a crash: 1. Launch the app and force a crash, which can be done from Settings → All Debug Options → Crash (fatal error) or similar. Note that Crash (CPU/Memory) does not appear to produce a crash log, and thus won’t trigger crash uploading. 2. Launch the app again (easiest with a debugger) 1. For the first crash of an app install: You will be prompted to opt in or out of crash reporting when the app is launched. Opt in and watch logs for “crcid” and you should see logs from CrashReportSender:56, and CrashCollection:95-109. 2. On subsequent crashes, when opted in, you should see statements confirming the received crcid was sent, and that the server returned either the same matching one, or a new one (in which case the new one should be stored and used on subsequent crash reports) To test clearing of the crcid when opting out: 1. Navigate to Settings → About and switch “Send Crash Reports” off, then back on again (this step should clear the crcid) 2. Follow steps from “To cause and report a crash” above, and confirm that the crash is submitted without an initial crcid, and that the server assigns one and it is stored (causing and uploading a second crash should confirm this new value is used on send).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please review the release process for BrowserServicesKit here.
Required:
Task/Issue URL: https://app.asana.com/0/1208592102886666/1208759541597499/f
iOS & macOS PRs: I’m looking for feedback on these BSK changes before posting app-level PRs
What kind of version bump will this require?: Minor (is this right? 😊).
CC: @LarryTurtis
Optional:
Tech Design URL: https://app.asana.com/0/1208592102886666/1208660326715650/f
Description:
DO NOT MERGE - this is a draft for input, not ready to go live yet. Before this is ready to merge:
Steps to test this PR:
static let reportServiceUrl = URL(string: "https://9e3c-20-75-144-152.ngrok-free.app/crash.js")!I have tested via the following means:
Note that I have not been able to test equivalent changes on macOS because I’m unable to add my computer to the DDG provisioning profile (see https://app.asana.com/0/1200194497630846/1208709596010744/f).
OS Testing:
—
Internal references:
Software Engineering Expectations
Technical Design Template